feat(ads): asset extension tools + RSA update + conversion-action mgmt#34
Open
illia-sapryga wants to merge 4 commits into
Open
feat(ads): asset extension tools + RSA update + conversion-action mgmt#34illia-sapryga wants to merge 4 commits into
illia-sapryga wants to merge 4 commits into
Conversation
4 tasks
illia-sapryga
added a commit
to illia-sapryga/adloop
that referenced
this pull request
May 6, 2026
The TestModulesUseDynamicEnums class was importing adloop.ads.conversion_actions and asserting on adloop.ads.write._VALID_PROMOTION_OCCASIONS — modules / constants that don't exist in this PR. They live in the follow-up PR (kLOsk#34, feat/asset-and-conversion-tools) and should be tested there. Branch A is intentionally minimal: the helper + helper-only unit tests. CI now passes — 12/12 tests.
kLOsk
pushed a commit
that referenced
this pull request
May 15, 2026
* feat(ads): dynamic Google Ads enum introspection helper
Adds adloop.ads.enums.enum_names() — pulls valid enum member names
straight from the google-ads SDK at the API version we're pinned to
(see adloop.ads.client.GOOGLE_ADS_API_VERSION).
Drops the need to hand-maintain parallel lists like:
_VALID_CONVERSION_ACTION_TYPES = {"AD_CALL", "WEBSITE_CALL", ...}
— which otherwise drift every time the SDK or API version updates.
The helper is module-cached (functools.lru_cache) so the no-auth
GoogleAdsClient used for introspection is built once per process,
and every enum_names() call after the first is essentially free.
UNSPECIFIED + UNKNOWN sentinels are dropped by default since they
are protobuf zero-values that should never appear in user input.
Pure addition. No existing validators changed in this PR; downstream
PRs (conversion-action tools, in-place asset updates, promotion
helper refactors) will switch their hardcoded enum sets to enum_names()
calls in their own commits.
12 unit tests cover:
- enum_names returns a frozenset
- UNSPECIFIED + UNKNOWN are excluded by default and includable on opt-in
- Critical members (AD_CALL, WEBSITE_CALL, GOOGLE_SEARCH_ATTRIBUTION_DATA_DRIVEN,
USE_RESOURCE_LEVEL_CALL_CONVERSION_ACTION, BLACK_FRIDAY) are present
- ConversionActionCountingTypeEnum returns exactly {ONE_PER_CLICK, MANY_PER_CLICK}
- Unknown enum names raise AttributeError
- LRU cache returns the same frozenset instance on repeat calls
- The introspection client is memoized
* test: remove cross-module enum tests from test_ads_enums.py
The TestModulesUseDynamicEnums class was importing adloop.ads.conversion_actions
and asserting on adloop.ads.write._VALID_PROMOTION_OCCASIONS — modules /
constants that don't exist in this PR. They live in the follow-up PR
(#34, feat/asset-and-conversion-tools) and should be tested there.
Branch A is intentionally minimal: the helper + helper-only unit tests.
CI now passes — 12/12 tests.
Adds the complete asset-extension and conversion-action management
surface that AdLoop was missing. Three logical chunks bundled here
because they share `ads/write.py`'s dispatch + apply infrastructure
and would conflict if split into separate PRs.
- `draft_call_asset` — campaign or customer scope, E.164 normalization,
ad-schedule restriction, optional conversion-action override
- `draft_location_asset` — Google Business Profile-backed AssetSet
(LOCATION_SYNC), with label/listing filters
- `draft_image_assets` — campaign image extensions from local files
with MIME + dimension validation
- `draft_callouts`, `draft_structured_snippets`, `draft_sitelinks` —
refactored to support BOTH campaign-scope AND customer-scope (the
account-level CustomerAsset path that propagates to every eligible
campaign automatically)
- `add_ad_schedule` — Mon-Sat 8am-9pm-style scheduling via
AdScheduleInfo CampaignCriterion
- `add_geo_exclusions` — negative geo CampaignCriterion records to
shrink a broad include list
- `_apply_assets()` shared helper routing a populate fn through either
CampaignAsset or CustomerAsset linkage based on scope
- Phone-number E.164 normalization with US/CA + EU trunk-prefix handling
- Update existing RSAs without delete-then-recreate
- Partial update via FieldMask — only the fields the caller passes
are modified
- Headlines/descriptions accept either bare strings (unpinned) or
{"text": "...", "pinned_field": "HEADLINE_1"} dicts (pinned)
- `draft_create_conversion_action` — AD_CALL / WEBSITE_CALL / WEBPAGE
/ GA4_CUSTOM with value, threshold, attribution model, counting type
- `draft_update_conversion_action` — partial update with FieldMask;
rename / promote-demote / set value / change duration threshold
- `draft_remove_conversion_action` — irreversible removal (warns that
SMART_CAMPAIGN_* and GOOGLE_HOSTED types reject mutation)
The 3 conversion-action tools live in their own module
`adloop/ads/conversion_actions.py` and route through dispatch via
`_apply_*_conversion_action_route` shims kept in `ads/write.py`.
- `link_asset_to_customer` — promote existing Asset rows from
campaign-scope to account-level (CustomerAsset)
- `update_call_asset` / `update_sitelink` / `update_callout` —
in-place asset updates with FieldMask
- `draft_promotion` / `update_promotion` — PromotionAsset create
+ swap (PromotionAsset is immutable; update is implemented as
create-new-link-old-unlink)
- Promotion module uses `enum_names("PromotionExtensionOccasionEnum")`
+ `enum_names("PromotionExtensionDiscountModifierEnum")` from
the dynamic-enums helper
- Conversion-actions module uses `enum_names("...")` for all 4
Google Ads enums it validates against — drops 4 hardcoded enum
sets that were drifting from the SDK
- Auto-cleanup script `scripts/cleanup_sitelink_links.py` for
duplicate sitelink CampaignAsset detection
- `tests/test_ads_extensions.py` — comprehensive validation +
apply-handler tests for every new function (uses fake services
mirroring the google-ads SDK protos; no network)
- `tests/test_conversion_actions.py` — 29 tests
- `tests/test_update_rsa.py` — RSA update integration tests
- All 430 tests pass
Add two new MCP tools for pushing offline conversion data directly to Google Ads, complementing the existing conversion-action management: - draft_upload_call_conversions: uploads call conversions (Caller ID + Call Start Time) to UPLOAD_CALLS-typed actions via ConversionUploadService.UploadCallConversions - draft_upload_enhanced_conversions_for_leads: uploads hashed PII to UPLOAD_CLICKS-typed actions via UploadClickConversions with user_identifiers populated. Works retroactively — no "action must exist before the conversion" constraint that UPLOAD_CALLS has. Both tools follow the AdLoop draft → confirm_and_apply pattern with plan storage, dry-run, audit logging, and per-row error reporting via partial_failure mode. Adds 25 new unit tests covering CSV parsing, action-name resolution, payload shape, partial-failure handling, and type-mismatch rejection (UPLOAD_CALLS vs UPLOAD_CLICKS). Also removes scripts/cleanup_sitelink_links.py, which was scoped to a specific client cleanup and shouldn't ship in the public tool.
bacb626 to
7666a41
Compare
Contributor
Author
|
Hey @kLOsk, just rebased this onto main and resolved the conflicts (12 commits behind → caught up). Also extended the PR with two new conversion-upload tools — If you'd prefer those upload tools split into a separate PR for easier review, happy to break them out. Otherwise, anything else you'd want before merging? |
This was referenced May 28, 2026
…es on create Two issues surfaced when creating ConversionActions via the Google Ads API that were not covered by validation: 1. include_in_conversions_metric is IMMUTABLE on create — Google derives it from the conversion category and rejects any value supplied in the create mutate with IMMUTABLE_FIELD. The apply function previously set it unconditionally, blocking every AD_CALL / PHONE_CALL_LEAD create with the default True value. Removed the line; callers who need to change it use draft_update_conversion_action after create. 2. default_value > 0 paired with always_use_default_value=False is rejected as INVALID_VALUE — it reads as "you set a value but told me not to use it". The draft tool defaults always_use_default_value to False, so the typical "set a default $250 lead value" call kept failing with no useful error. Added an auto-correct in the draft function: when default_value > 0 and the caller didn't explicitly opt out, flip always_use_default_value to True. Tests added cover both regressions plus a sanity test that callers who already set the flag correctly still get their value through, and that default_value=0 keeps the flag False (for snippet-value callers). All 227 tests pass.
draft_call_asset previously supported only customer or campaign scope.
This blocks the multi-service / multi-ad-group pattern where each ad
group inside one campaign needs its own tracked phone number wired to
its own service-specific call conversion action (e.g. Tint ad group
fires "Tint Call from Ad" worth $400, PPF ad group fires "PPF Call
from Ad" worth $2,000).
Added an ad_group_id parameter to both the MCP wrapper in server.py
and the impl in ads/write.py, with most-specific-wins scope precedence
(ad_group > campaign > customer). When ad_group_id is provided, the
apply function creates an AdGroupAsset link instead of a CampaignAsset
or CustomerAsset link.
Tests added:
- draft test: ad_group_id selects ad_group scope and populates entity
- draft test: ad_group_id wins over campaign_id (precedence rule)
- apply test: creates an AdGroupAsset link with correct ad_group path
and conversion action wiring
- apply test: missing ad_group_id raises a clear error
Also fixed a latent bug in _FakeGoogleAdsService.ad_group_path: it
inherited from _FakePathService which uses a single prefix
("campaigns") for all resource path helpers, so ad_group_path returned
"customers/.../campaigns/{id}" instead of "customers/.../adGroups/{id}".
Override returns the correct prefix; only affects the new ad-group
scope test, no existing tests touched ad_group_path.
NOTE: when adding new params to MCP tools you must update BOTH the
impl in ads/*.py AND the wrapper in server.py — the wrapper's signature
is what FastMCP uses to build the JSON schema exposed to clients. The
first patch attempt only updated the impl and broke at call time with
unexpected_keyword_argument.
All 227 tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three logical chunks bundled here because they share
ads/write.py's dispatch + apply infrastructure and would conflict at the file level if split into separate PRs.update_responsive_search_ad)Depends on #33 (dynamic Google Ads enum introspection) — please merge that first.
What's added
1. Asset extension tools
draft_call_asset+19164609257).draft_location_assetLOCATION_SYNCAssetSet, with label/listing filtersdraft_image_assetsdraft_calloutsCustomerAssetpath that propagates to every eligible campaign automatically)draft_sitelinksdraft_structured_snippetsadd_ad_scheduleAdScheduleInfo CampaignCriterionadd_geo_exclusionsCampaignCriterionrecords to shrink a broad include listlink_asset_to_customerCustomerAsset) — for re-using shared logos / sitelinks across campaignsupdate_call_asset/update_sitelink/update_calloutFieldMask— only the fields the caller passes are modifieddraft_promotion/update_promotionPromotionAssetcreate + swap (PromotionAsset is immutable, so 'update' is implemented as create-new-link-old-unlink atomically)_apply_assets()CampaignAssetorCustomerAssetlinkage based on scope_normalize_phone_e164()2. RSA in-place update
update_responsive_search_adFieldMask— only the fields the caller passes are modified. Headlines/descriptions accept either bare strings (unpinned) or{\"text\": ..., \"pinned_field\": \"HEADLINE_1\"}dicts (pinned).3. Conversion-action management (3 new tools)
draft_create_conversion_actiondraft_update_conversion_actionFieldMask— rename / promote-to-Primary / demote-to-Secondary / set value / change duration threshold / change attributiondraft_remove_conversion_actionSMART_CAMPAIGN_*andGOOGLE_HOSTEDtypes reject mutation withMUTATE_NOT_ALLOWED(Google manages those).The 3 conversion-action tools live in their own module
adloop/ads/conversion_actions.pyand route through_execute_plan's dispatch via lazy-import_apply_*_conversion_action_routeshims.Refactors (uses #33's
enum_names())The
_VALID_TYPES,_VALID_CATEGORIES,_VALID_COUNTING_TYPES,_VALID_ATTRIBUTION_MODELSsets inconversion_actions.pyare pulled dynamically from the SDK viaenum_names(). Same for_VALID_PROMOTION_OCCASIONS,_VALID_DISCOUNT_MODIFIERS,_VALID_CALL_REPORTING_STATESinads/write.py.The hardcoded versions of those sets had drifted from the SDK. The
ConversionActionTypeEnumset was missing 29 of 40 valid values;PromotionExtensionOccasionEnumwas missing 2.Validated end-to-end
This batch was used to build out a real Google Ads account end-to-end (BGI Tint, customer 8202753856):
Calls from Ads (≥90s)AD_CALL @ $250,Website Call (GFN ≥90s)WEBSITE_CALL @ $250 with 90s threshold) and bound them to a custom conversion goalUSE_RESOURCE_LEVEL_CALL_CONVERSION_ACTIONTest plan
uv run pytest→ 430/430 teststests/test_ads_extensions.py— comprehensive validation + apply-handler tests for every new function (uses fake services mirroring the google-ads SDK protos; no network)tests/test_conversion_actions.py— 29 tests covering validation, partial update mask, FieldMask correctness, MCP registrationtests/test_update_rsa.py— RSA update integration tests_apply_*handlers verified to emit the rightFieldMaskpaths via fake service inspectionKnown follow-ups (out of scope for this PR)
ads/write.py(_VALID_BIDDING_STRATEGIES,_VALID_CHANNEL_TYPES,_VALID_DAYS_OF_WEEK,_VALID_HEADLINE_PINS, etc.) could be migrated toenum_names()in a separate refactor PR.update_call_assetallows full-list replacement ofad_schedule_targetsbut no append-mode — append would be useful for some workflows.